Hello @blimlim! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-11-18 01:13:45 UTC

coverage: 58.624% (+0.3%) from 58.361%
when pulling d4e3334 on ACCESS-NRI:527-compress-model-logs
into 7c885b8 on payu-org:master.

I'd be tempted to add compress_log_files as a sub-option to archive. Something like

archive:
    enable: True
    compress_logs: True

and make compress_logs default to True.

Currently archive is a boolean, but we can support it being a dict as well. c.f. the collate option, which is converted to a dict from a boolean configuration option:

It would also require changing the code to check if archive is enabled to something like this

I'd be tempted to add compress_log_files as a sub-option to archive

I've had a go at implementing this in 0b2fcc9. See comments for further details!

Currently cice4 in ESM1.5 is the main model causing problems, but there are a couple of options for generalising this, making it available to other models:

1. Just move the three methods cice.compression_enabled(), cice.get_log_files(), cice.compress_log_files() from the cice.py driver to the model.py driver, and add self.logs_to_compress = [] to the general model.__init__. Compression can be added to other models by adding the required regex patterns to self.logs_to_compress = [] in the relevant model driver.

2. Leave the methods in cice.py, add methods with the same name to model.py that just pass. Compression can then be implemented in other models by overriding these methods in the relevant driver.

I think the methods should work generally enough that option 1 would work, and I think this would be a bit simpler/easier to follow. Keen to hear other ideas or opinions though!

@aidanheerdegen I've just marked this one as ready to review! I think the main outstanding question is whether we are happy with the log compression being implemented in the cice.py driver to start off with, or if we prefer to have it more general.